Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/icaptcha.rs`:
- Around line 51-56: The parse_mode helper in icaptcha.rs currently maps any
unrecognized ICAPTCHA_MODE value to Mode::Off, which silently disables
enforcement. Update parse_mode (and the other call site referenced by the
comment) so invalid inputs are detected and surfaced with a warning or error log
before falling back, using the existing Mode enum and the icaptcha configuration
path to keep operator-visible signal for typos like “enforced”.
- Around line 98-101: The startup key fetch in fetch_key is currently using
reqwest::get without any timeout, so a hung /v1/pubkey endpoint can block
icaptcha::init during startup. Update fetch_key to use a reqwest client or
request builder with a timeout for the pubkey request, and keep the existing
fallback behavior so init can return inert when the fetch does not complete.
Make the change in the fetch_key path used by icaptcha::init and main startup,
preserving the current JWKS parsing and decode_key flow.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.env.example:
- Around line 98-105: Reorder the iCaptcha environment entries in the example so
dotenv-linter no longer flags them: move ICAPTCHA_PUBKEY and
ICAPTCHA_REQUIRED_LEVEL above ICAPTCHA_URL while keeping the existing
ICAPTCHA_MODE and related comments intact. Update the .env.example block in
place so the key ordering is consistent with the linter’s expected
alphabetical-style grouping for the ICAPTCHA_* variables.

Findings

• [P2] Complete CodeRabbit's request to surface invalid iCaptcha modes
  crates/gitlawb-node/src/icaptcha.rs:51
  CodeRabbit's inline request is still valid: parse_mode maps every unrecognized ICAPTCHA_MODE value to Mode::Off, so an operator typo such as ICAPTCHA_MODE=enforced silently disables the protection they were trying to enable. Please complete that review request by accepting only the documented values and logging or otherwise surfacing invalid input before falling back.

• [P2] Complete CodeRabbit's request to bound startup key fetches
  crates/gitlawb-node/src/icaptcha.rs:98
  CodeRabbit's startup-availability request is still valid: main awaits icaptcha::init(), and fetch_key uses reqwest::get() without a timeout. With ICAPTCHA_MODE=shadow or enforce, a hung /v1/pubkey endpoint can leave the node stuck during startup instead of reaching the intended fail-safe inert path. Please complete that review request by adding a bounded request timeout while preserving the current fallback behavior.

• [P2] Surface invalid required-level configuration instead of lowering to the default
  crates/gitlawb-node/src/icaptcha.rs:110
ICAPTCHA_REQUIRED_LEVEL is parsed with .and_then(|v| v.parse().ok()).unwrap_or(3), so an operator trying to raise the gate with a typo such as ICAPTCHA_REQUIRED_LEVEL=5 or ICAPTCHA_REQUIRED_LEVEL=five silently runs at level 3. In enforce mode that weakens the configured challenge threshold with no warning. Please validate and warn or reject invalid level values instead of defaulting silently.

• [P3] Complete CodeRabbit's dotenv-linter request for the new env block
  .env.example:101
  CodeRabbit's later lint comment is still current: the new block leaves ICAPTCHA_URL before ICAPTCHA_PUBKEY and ICAPTCHA_REQUIRED_LEVEL, and its dotenv-linter output flags both as UnorderedKey. Please complete that review request by reordering the new ICAPTCHA_* entries so the example stops carrying the lint warning.

Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.

Findings

• [P2] Consume proof IDs so one solved challenge cannot be replayed
  crates/gitlawb-node/src/icaptcha.rs:230
  The verifier decodes the signed claims and checks exp, level, and sub, but it does not read or store the proof's jti/nonce, even though the captured proof fixture includes one. That means the same X-ICaptcha-Proof can be reused repeatedly by the same authenticated DID until it expires, including across the spam-prone create_repo and register routes this PR is trying to gate. With the existing per-DID throttle, one solved challenge can still cover every allowed creation request in the window, and new-DID registration remains a replayable bearer proof for that DID. Please consume a unique proof ID, at least until its exp, and reject repeats so enforce actually requires a fresh earned proof for each gated creation action.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/icaptcha.rs`:
- Around line 184-205: The `check` flow in `icaptcha.rs` consumes the proof
`jti` too early, before the request is fully known to be admissible. Update
`check`/`Decision::Consume` handling so `register` and `create_repo` can verify
the proof first and only call the consumption path after DID/name/existence
validation has passed, or split verification from consumption entirely. Use the
existing `check`, `decide`, `Decision::Consume`, and `db.consume_proof_jti`
symbols to keep the replay protection but avoid burning valid proofs on rejected
requests.

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 881-882: The webhook payload is hard-coding the ref update
metadata as not forced, which misreports force-pushes to downstream consumers.
Update the payload construction in the repo webhook handling logic around the
ref update object so the forced flag is computed from the old/new SHAs instead
of always set to false. Use the existing update context in repos.rs to determine
whether the change is a non-fast-forward update (for example, checking that both
SHAs are non-zero and the old commit is not an ancestor of the new one) before
setting the forced field.

In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 785-795: The icaptcha replay ledger is keyed only by jti, so a
proof consumed in one gated action blocks reuse in another even though the
contract is per action. Update the icaptcha_consumed_proofs schema and the
related APIs in crate::icaptcha::check to include an action discriminator (for
example register vs create_repo) and enforce uniqueness on the composite key,
then adjust the insert/lookup logic to scope replay checks by both action and
jti.

---

Outside diff comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 145-165: Move the iCaptcha proof consumption later in the repo
creation flow so cheap request validation happens first. In the create-repo path
in `repos.rs`, keep the repo name sanitization and `state.db.get_repo` existence
check before calling `crate::icaptcha::check`, then only consume the JTI once
the request is known to be valid and non-duplicate. Use the `icaptcha::check`
call, `owner_did`, and `state.db.get_repo` as the key anchors when reordering
the logic.

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 881-882: The webhook payload is hard-coding the ref update
metadata as not forced, which misreports force-pushes to downstream consumers.
Update the payload construction in the repo webhook handling logic around the
ref update object so the forced flag is computed from the old/new SHAs instead
of always set to false. Use the existing update context in repos.rs to determine
whether the change is a non-fast-forward update (for example, checking that both
SHAs are non-zero and the old commit is not an ancestor of the new one) before
setting the forced field.

In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 785-795: The icaptcha replay ledger is keyed only by jti, so a
proof consumed in one gated action blocks reuse in another even though the
contract is per action. Update the icaptcha_consumed_proofs schema and the
related APIs in crate::icaptcha::check to include an action discriminator (for
example register vs create_repo) and enforce uniqueness on the composite key,
then adjust the insert/lookup logic to scope replay checks by both action and
jti.

---

Outside diff comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 145-165: Move the iCaptcha proof consumption later in the repo
creation flow so cheap request validation happens first. In the create-repo path
in `repos.rs`, keep the repo name sanitization and `state.db.get_repo` existence
check before calling `crate::icaptcha::check`, then only consume the JTI once
the request is known to be valid and non-duplicate. Use the `icaptcha::check`
call, `owner_did`, and `state.db.get_repo` as the key anchors when reordering
the logic.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/icaptcha.rs`:
- Around line 200-205: The iCaptcha failure paths in consume and the other
matching branch are still returning AppError::Unauthorized, which gets mapped to
the not_an_agent code. Update the iCaptcha flow in icaptcha::consume to use a
dedicated AppError variant for missing/replayed proofs, and add the
corresponding mapping in error.rs so these cases return the new
iCaptcha-specific API code instead of the authn one.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/icaptcha.rs`:
- Around line 221-224: Shadow mode is dropping the verified proof ID, so replay
detection never runs in the “would reject” path. Update the flow around decide,
ProofGuard, and the shadow-mode handling in the verification path so a
successful shadow verification still carries the jti forward instead of
collapsing to ProofGuard(None). Then make register and create_repo route that
proof through a read-only replay check/log path, preserving consume behavior
only for enforce mode and leaving shadow mode to report reused jtis without
mutating state.

Reviewed the merged state at 66e7a34 (CI green, merge clean). The core gate is sound and I confirmed the load-bearing parts by execution: Ed25519 verify_strict, the proof-to-DID binding via the shared did_matches normalizer (handles short/full did:key, not a bare ==), atomic single-use through INSERT ... ON CONFLICT DO NOTHING, the verify-then-consume split, and that the gate is additive to the existing owner/auth checks (INV-1 intact). Migration 8 is the correct next version, additive and idempotent. The CodeRabbit threads and the two earlier change requests are all addressed on this head, including the shadow read-only replay path.

Two things to close before merge, then a few notes.

Findings

• [P2] Gate fork_repo, or state why it is exempt
  crates/gitlawb-node/src/api/repos.rs:1420
fork_repo creates a repo (db.create_repo after a git clone --mirror) and never calls icaptcha::verify_request; its handler signature does not even take HeaderMap. Under enforce an agent bypasses the create_repo/register gate entirely by forking. It is the third repo-creation entrypoint and the one that previously drifted on visibility (#98). create_repo and register are gated and GraphQL exposes no creation mutation, so fork is the only open path. Either thread the gate through it (verify before the clone, consume just before db.create_repo) or document the exemption with the reasoning.

• [P2] Add DB-level tests for the single-use ledger
  crates/gitlawb-node/src/db/mod.rs:1119
consume_proof_jti, check_proof_jti, sweep_expired_proofs, and ProofGuard::consume have no execution coverage, although a #[sqlx::test] harness already exists in this same file. Single use is the feature's security value, and right now nothing runs it. The path itself is correct (I checked: first consume succeeds, replay returns false, check_proof_jti does not mutate, sweep keeps still-valid rows and drops expired), so this is just landing those as committed tests.

Notes

• [P3] Document ICAPTCHA_PUBKEY as the supported enforce configuration
  crates/gitlawb-node/src/icaptcha.rs:246
decide() returns Allow when no key loaded, so an unreachable /v1/pubkey at startup degrades enforce to inert with only a warn. That is a reasonable availability choice, but it means enforce fails open on a startup network blip. Pinning ICAPTCHA_PUBKEY removes the startup fetch dependency; worth calling it out as the way to run enforce.

• [P3] Consider 403 rather than 401 for IcaptchaProofRequired
crates/gitlawb-node/src/error.rs:60
  The caller is already HTTP-signature authenticated; what is missing is an anti-abuse proof, which is closer to 403 Forbidden than 401. The distinct icaptcha_proof_required code is right; only the status is debatable. Confirm no client keys on the numeric 401 before changing it.

• [P3] Confirm the challenge has a headless solve path before flipping enforce
  The verification side is agent-clean (header-carried, offline, DID-bound). But if icaptcha.gitlawb.com requires interactive human solving, enabling enforce locks autonomous agents out of repo creation and registration, which are the two core write paths. Worth stating the programmatic acquisition flow in the PR or a linked doc.

• [P3] Pure-shadow replay detection is effectively inert
  crates/gitlawb-node/src/icaptcha.rs
  Shadow now routes to the read-only check_proof_jti, but shadow never writes the ledger, so in a shadow-only deployment nothing is ever recorded and the replay warning cannot fire. It only does anything in a mixed shadow+enforce setup. Minor, just flagging the limited value of the new path on its own.

Core is solid; the only real gap is the ungated fork path. Once fork_repo is covered (or consciously exempted) and the ledger has tests, this is good to go.

[P2] Gate fork_repo, or explicitly document why it is exempt
crates/gitlawb-node/src/api/repos.rs:1420
fork_repo still creates a repository with state.db.create_repo(&record).await?, but unlike create_repo and register its handler does not take HeaderMap and never calls crate::icaptcha::verify_request. In ICAPTCHA_MODE=enforce, an authenticated agent can therefore bypass the new anti-abuse gate by forking any readable repo instead of calling POST /api/v1/repos, even though this route is in the same rate-limited creation route group and produces a new repo row. Please either thread the iCaptcha proof through this path as well, consuming it immediately before db.create_repo, or document the intentional exemption and why fork creation is not part of this abuse boundary.

[P2] Add execution coverage for the replay ledger
crates/gitlawb-node/src/db/mod.rs:1119
The security value of the gate now depends on the new DB-backed single-use ledger, but the committed tests only exercise the pure verifier/decision logic. consume_proof_jti, check_proof_jti, sweep_expired_proofs, and the ProofGuard::consume path are not executed by any committed test even though this file already has a #[sqlx::test] harness. Please add DB-level coverage that proves first consume succeeds, replay returns false, the shadow read-only check does not mutate state, and sweeping removes only expired proof rows.

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 1132-1143: `check_proof_jti` is currently read-only, so
shadow-mode requests never get recorded and repeated proofs keep appearing
fresh. Update the shadow-mode caller that uses `check_proof_jti` to first record
the proof as consumed on the first seen request, then use the check only to
decide whether to log a would-be replay rejection while still allowing the
request through. Keep the replay-detection behavior centered around
`check_proof_jti` and the downstream shadow path so the first-seen proof is
persisted before subsequent shadow checks.

In `@crates/gitlawb-node/src/icaptcha.rs`:
- Around line 207-224: In `icaptcha::consume`, the shadow-mode branch currently
uses `db.check_proof_jti(...).await?`, which lets transient DB errors bubble up
and fail `register`/`create_repo` even though shadow mode should only log.
Change the `else` path to handle `check_proof_jti` errors locally: log the
failure with `tracing` (including `job.jti`), treat it as non-fatal, and
continue returning `Ok(())` so shadow mode never fails closed; keep the enforced
replay rejection path unchanged.

[P2] Complete the request to gate fork_repo or document its exemption
crates/gitlawb-node/src/api/repos.rs:1420
Jatmn's change request is still valid on the current head: fork_repo is in the same rate-limited creation route group as create_repo and register, and it still creates a new repo row with state.db.create_repo(&record).await?, but the handler does not take HeaderMap and never calls crate::icaptcha::verify_request. In ICAPTCHA_MODE=enforce, an authenticated agent can bypass the anti-abuse gate by forking any readable repo instead of calling POST /api/v1/repos. Please complete that review request by threading the iCaptcha proof through this path and consuming it immediately before db.create_repo, or explicitly document why fork creation is outside this abuse boundary.

[P2] Add committed execution coverage for the replay ledger
crates/gitlawb-node/src/db/mod.rs:1119
Jatmn's replay-ledger coverage request is also still open. The committed tests exercise the pure verifier/decision logic, but consume_proof_jti, check_proof_jti, sweep_expired_proofs, and the ProofGuard::consume path are not executed by any committed test even though the file already has a #[sqlx::test] harness. Because the single-use DB ledger is the security boundary that makes proofs non-replayable, please add DB-level tests proving first consume succeeds, replay returns false, the shadow observation path behaves as intended, and sweeping removes only expired proof rows.

[P2] Complete CodeRabbit's request to make shadow replay detection record first-seen proofs
crates/gitlawb-node/src/icaptcha.rs:218
CodeRabbit's current shadow-mode replay request is still valid: the shadow path calls db.check_proof_jti(&job.jti).await?, but that helper is read-only, so a shadow-only deployment never records the first use of a proof. A repeated proof therefore keeps appearing fresh and never produces the "would reject replay" signal that shadow mode is supposed to provide before operators flip to enforce. Please complete that request by recording first-seen proof IDs in the shadow observation path while still allowing the request through.

[P2] Complete CodeRabbit's request to keep shadow replay-check errors non-fatal
crates/gitlawb-node/src/icaptcha.rs:218
The same shadow branch still propagates db.check_proof_jti(...) with ?. Since both gated handlers call proof.consume(&state.db).await?, a transient DB error in shadow mode rejects register or create_repo, even though shadow mode is documented as an observe/log mode that always allows. Please handle replay-check failures locally in the shadow branch, log the jti and error, and return Ok(()) so only enforce mode fails closed on ledger errors.